-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Partial fix and workaround for #2400 #2429
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2429 +/- ##
==========================================
- Coverage 73.96% 73.96% -0.01%
==========================================
Files 484 484
Lines 245455 245458 +3
==========================================
+ Hits 181544 181546 +2
- Misses 63911 63912 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/magma.gi
Outdated
@@ -1209,6 +1209,9 @@ InstallMethod( AsMagma, | |||
D := AsSSortedList( D ); | |||
L := ShallowCopy( D ); | |||
M := Submagma( MagmaByGenerators( D ), [] ); | |||
if IsGroup(M) then | |||
M:=Submagma(MagmaByGenerators(D),[One(ElementsFamily(FamilyObj(D)))]); | |||
fi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were to look at this code in a year, I'd be completely confused as to what it's about. Even if I spend the extra effort to use git annotate
to find the commit message and look at it, I don't think I'd be much wiser: "... replacing the empty submagma with the one-submagma, if it turns out a group" -- Huh? How can the submagma turn out to be a group, if it is empty? Isn't it a bug that it's marked as a group in the first place? Or is the bug that it's marked as a group"?
In fact, reading up on #2400 again, I am not convinced this change is correct. Rather, it looks like a workaround for a deeper issue. As such, I don't feel comfortable merging it, until somebody explains why it is correct after all (and ideally that explanation then should be part of a comment before this if/fi
block).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is a workaround (and I've added a comment to this effect). The problem is that this bug arises in test suites (in examples of considering groups as magmas) and has the potential of holding up other commits, while it is completely unclear to me how one would fix this.
@@ -175,7 +175,7 @@ InstallGlobalFunction( SubadditiveMagmaNC, function( M, gens ) | |||
if IsEmpty( gens ) then | |||
K:= NewType( FamilyObj(M), | |||
IsAdditiveMagma | |||
and IsTrivial | |||
and IsEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this change.
1) Wrong `IsTrivial` set in empty submagma 2) Fix `Submagma` method by replacing the empty submagma with the one-submagma, if it turns out a group. (Work around bug, not a fix) This resolves gap-system#2400 as far as groups are concerned.
A proper fix for the second issue would be this (as described by @ThomasBreuer on issue #2400): diff --git a/lib/magma.gd b/lib/magma.gd
index d939bcd62..6577c0921 100644
--- a/lib/magma.gd
+++ b/lib/magma.gd
@@ -130,7 +130,7 @@ DeclareCategory( "IsMagmaWithInverses",
and IsMultiplicativeElementWithInverseCollection );
InstallTrueMethod( IsMagmaWithInverses,
- IsFiniteOrderElementCollection and IsMagma );
+ IsFiniteOrderElementCollection and IsMagmaWithOne );
#############################################################################
## In theory, that would loose some features, but in practice, this does not seem to matter, at least for the testsuite. We could also add a comment saying e.g.
I also have a local patch which adds |
As requested made this a PR of its own. It avoids creating magmas from group elements that have the wrong order.
Tests for this are already in the tst files (therefore no extra test provided here) but will be triggered only once the new method for SetSize in #2387 has been merged.